This is due to the fact that we install a listener for each focusable component and that listener runs 2 times for every key press (once on keydown and again on keyup), in extreme scenarios we can have 200+ listeners attached.

I assume this means our keyboard and focus listeners? We only attach one of each listener and use a subscription to update anything which calls useFocusVisible. So there shouldn't be 200+ listeners attached.

If it's a listener that you are adding, could you follow the same model as us and use a subscription model?

Otherwise, maybe an example of how this is all interacting would be useful, in a codesandbox or stackblitz. Or some tests or stories which show that many listeners are being added.

Handler === listener here, I updated my PR description, we are adding 200 items to changeHandlers.

The important thing is we are executing 200 functions in a very hot code path, 2x for every keystroke and each one of those invocations runs 4 identical instanceof checks, it's very inefficient.

I can setup a repro, but you can tell from the code pretty easily that it will add a handler unconditionally for each component instance that calls useFocusRing().

The changes in this PR are pretty reasonable I feel and all the existing tests pass (useFocusRing code paths have good test coverage). Is there concern with the change?

Here is the repro:

https://codesandbox.io/p/sandbox/vy3rxw?file=%2Fsrc%2FApp.js%3A15%2C1

If you add a breakpoint, you can see that changeHandlers has 1001 items in it (one for each focusable component instance).

It should be noted that we were not ourselves able to repro the same perf issues the customer was reporting but this is objectively a lot of unnecessary overhead that can be optimized regardless.

I see now, thanks, got a little hung up on the adding listeners, but it was adding subscriptions.

It looks like we got close to this a while back #1516

I'm trying to determine why we didn't do this previously when it was partially suggested. Could be as simple though as not having a use case that extreme at the time.

@snowystinger anything else we need to do to get this merged?

Yep, a second review, thanks for your patience